Skip to content

fix: null-reference crash when a Where predicate null-checks a captured value#122

Merged
Tr00d merged 5 commits into
masterfrom
guillaumefaas/sdk-1232-null-reference-crash-on-linq-where-null-check
Jul 9, 2026
Merged

fix: null-reference crash when a Where predicate null-checks a captured value#122
Tr00d merged 5 commits into
masterfrom
guillaumefaas/sdk-1232-null-reference-crash-on-linq-where-null-check

Conversation

@Tr00d

@Tr00d Tr00d commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Fixes supabase-community/supabase-csharp#192

Problem

A common C# idiom for an optional filter crashes on Get:

await client
    .From<TTable>()
    .Select("*")
    .Where(x => requestModel.FilterPredicate == null || requestModel.FilterPredicate(x)) // Boom!
    .Get();

Three defects combine in the Where translation:

  1. The crash. For ||/&&, WhereExpressionVisitor combined sub-visitor results with the null-forgiving operator. A branch it has no handler for — here the delegate invocation FilterPredicate(x) — left its sub-filter null, which was stuffed into the nested criteria list and dereferenced later by Table.PrepareFilter during GenerateUrl(), producing the exact NullReferenceException stack trace in the issue, far from the user's code.
  2. Closure expressions were mistranslated. requestModel.FilterPredicate == null doesn't reference the model at all, yet it was translated as if FilterPredicate were a column, producing a bogus FilterPredicate=eq. filter.
  3. Nested null checks were broken. The == nullis.null conversion lived in Table.Where and only applied at the top level; inside ||/&& (e.g. x => x.Name == null || x.Name == "foo") it emitted an invalid eq. filter.

Fix

  • The visitor now tracks the lambda parameter and evaluates parameter-free branches locally, folding constants through ||/&& with C#'s short-circuit semantics. The reported idiom works: when the delegate is null, FilterPredicate == null folds to true, the right side is never touched, and Where applies no filter — exactly what the reporter expected.
  • A branch that references the model but cannot be translated (a compiled delegate is opaque IL — it can never become a PostgREST query) now throws a descriptive ArgumentException pointing at the conditional-query workaround, instead of an NRE at Get.
  • An always-false predicate throws too: silently matching all rows would be dangerous for Update/Delete.
  • The null-check conversion moved into the visitor, so is.null/not.is.null is emitted at any nesting depth.
  • PrepareFilter guards against null nested filters reachable via the public QueryFilter(Operator.And/Or, …) constructor with a clear exception.

Deliberately out of scope: the other known LINQ-provider gaps (closure-list Contains, bool member columns, column-vs-column comparisons) — same family, separate issues.

Tests (written first, red → green)

New tests in LinqTests assert on GenerateUrl() (no live instance needed):

Test Before (red) After (green)
Null-check on absent delegate (the issue's repro) NullReferenceException, identical stack trace to the issue no filter applied
Null-check on present delegate no exception, invalid filter descriptive ArgumentException
Always-false predicate no exception, invalid filter descriptive ArgumentException
Nested column null-check invalid name.eq. filter or=(name.is.null,name.eq.…)

Full suite against a fresh supabase start stack: 96/96 pass (rebased on #121).

Tr00d and others added 5 commits July 8, 2026 14:51
…ed value

A LINQ predicate like `x => filterPredicate == null || filterPredicate(x)`
crashed with a NullReferenceException on `Get`: branches the visitor could
not translate (i.e. a delegate invocation) were combined with the
null-forgiving operator, putting a literal null into the nested filter
criteria that `PrepareFilter` later dereferenced.

The visitor now tracks the lambda parameter and evaluates branches that
never reference the model locally, folding them through `||`/`&&` with
C#'s short-circuit semantics. The reported idiom therefore works: when
the captured delegate is null the predicate is always true and `Where`
applies no filter. Branches that do reference the model but cannot be
translated (a compiled delegate is opaque) now throw a descriptive
ArgumentException pointing at the conditional-query workaround, as does
an always-false predicate (silently matching every row would be dangerous
for `Update`/`Delete`).

Also moves the `== null` -> `is.null` / `!= null` -> `not.is.null`
conversion from `Table.Where` (top level only) into the visitor so nested
null checks like `x => x.Name == null || x.Name == "foo"` no longer emit
an invalid `eq.` filter, and guards `PrepareFilter` against null nested
filters supplied through the public QueryFilter constructor.

Fixes supabase-community/supabase-csharp#192

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
@Tr00d Tr00d marked this pull request as ready for review July 8, 2026 14:29
Comment on lines +26 to +27
private WhereExpressionVisitor(ParameterExpression? parameter) =>
this.parameter = parameter;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can't you use a primary constructor here?

@Tr00d Tr00d Jul 9, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@spydon A couple of reasons:

  • The SDK isn't on the latest C# version yet, and the current version doesn't support primary constructors.
  • Encapsulation: with a primary constructor, any external component could set the ParameterExpression. Currently, only this Visitor can do so (internally).
  • It would be bit ugly with nullability and a default value: internal class WhereExpressionVisitor(ParameterExpression? parameter = null) 🤮 There's a bit of semantics/intent too as it would imply ParameterExpression is something expected

I'll have a fun time getting rid of nullability 😅

EDIT: I could upgrade the langversion or go through a deeper rework of the LINQ visitor, but I want this PR to address the bug first, and not refactor the whole thing. These will come later (soon enough); the multi-repo approach is just a pain to deal with atm

@Tr00d Tr00d merged commit 5581287 into master Jul 9, 2026
1 check passed
@Tr00d Tr00d deleted the guillaumefaas/sdk-1232-null-reference-crash-on-linq-where-null-check branch July 9, 2026 05:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Get throwing Null Reference when one of the conditions in Where is a null validation

3 participants